Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate fields for APM package #4432

Merged
merged 21 commits into from
Nov 25, 2020
Merged

Conversation

jalvz
Copy link
Contributor

@jalvz jalvz commented Nov 17, 2020

As title. For more details, head to the diff in the Readme.md file.

Motivation/summary

Checklist

I have considered changes for:

How to test these changes

  • run make update
  • check the contents of apmpackage/apm

Related issues

#4004

@apmmachine
Copy link
Collaborator

apmmachine commented Nov 17, 2020

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #4432 updated]

  • Start Time: 2020-11-25T08:51:20.964+0000

  • Duration: 40 min 25 sec

Test stats 🧪

Test Results
Failed 0
Passed 4754
Skipped 143
Total 4897

Steps errors 3

Expand to view the steps failures

Compress

  • Took 0 min 0 sec . View more details on here
  • Description: tar --exclude=coverage-files.tgz -czf coverage-files.tgz coverage

Compress

  • Took 0 min 0 sec . View more details on here
  • Description: tar --exclude=system-tests-linux-files.tgz -czf system-tests-linux-files.tgz system-tests

Test Sync

  • Took 3 min 49 sec . View more details on here
  • Description: ./.ci/scripts/sync.sh

Copy link
Contributor

@simitt simitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM on a first review. My main question is how this integrates with any update workflow - will it be part of running make update?

Do you plan on addressing the TODOs before labeling this ready for review? (I believe fetching ECS info from github rather than expecting local installation would make sense).

apmpackage/genfields.go Outdated Show resolved Hide resolved
apmpackage/model.go Outdated Show resolved Hide resolved
apmpackage/model.go Outdated Show resolved Hide resolved
apmpackage/model.go Outdated Show resolved Hide resolved
@jalvz
Copy link
Contributor Author

jalvz commented Nov 18, 2020

Overall LGTM on a first review. My main question is how this integrates with any update workflow - will it be part of running make update?

Exactly, it should be part of the target that generates the big fields.yml file.

Do you plan on addressing the TODOs before labeling this ready for review? (I believe fetching ECS info from github rather than expecting local installation would make sense).

Thanks for validating, I wasn't sure (I copied the loadECSFields method from the integrations repo, that expects a local install, that's why) So yeah, will do that

@jalvz jalvz marked this pull request as draft November 20, 2020 14:47
@jalvz jalvz mentioned this pull request Nov 23, 2020
15 tasks
@jalvz jalvz requested review from simitt and axw November 23, 2020 14:43
@jalvz jalvz marked this pull request as ready for review November 23, 2020 14:43
apmpackage/apm/0.1.0/manifest.yml Show resolved Hide resolved
var packageVersion string

func main() {
flag.StringVar(&packageVersion, "packageVersion", "0.1.0", "Package version")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a TODO to get the default version from cmd/version.go (or move that into a package and make it an exported variable)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, lmk if i got it right

apmpackage/gendocs.go Outdated Show resolved Hide resolved
apmpackage/gendocs.go Outdated Show resolved Hide resolved
apmpackage/model.go Outdated Show resolved Hide resolved
apmpackage/model.go Outdated Show resolved Hide resolved
apmpackage/model.go Outdated Show resolved Hide resolved
tests/system/apmserver.py Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
@axw
Copy link
Member

axw commented Nov 24, 2020

A couple of comment which I forgot to add:

  • I wonder if there's value in checking in the generated files, vs. just checking in the script and static files. We can generate into the build directory, and copy from there into the integrations repo.
  • It would be nice to cache the (versioned) ecs_flat.yml files in the repo, so we don't have to download them every time we run make update.

@jalvz
Copy link
Contributor Author

jalvz commented Nov 24, 2020

I wonder if there's value in checking in the generated files

I think there is. Eg. when new fields are added is easy to see in the diff the table generated for the readme file and check it is rendered properly.

I don't have a strong opinion thou.

It would be nice to cache the (versioned) ecs_flat.yml files in the repo, so we don't have to download them every time we run make update.

I can add a TODO for that, but not sure that is worth the trouble, tbh. It takes millisecs and we need an internet connection to push the changes anyways.

@axw
Copy link
Member

axw commented Nov 24, 2020

I think there is. Eg. when new fields are added is easy to see in the diff the table generated for the readme file and check it is rendered properly.

I don't have a strong opinion thou.

Same, so keep it for now and maybe we'll revisit it later.

I can add a TODO for that, but not sure that is worth the trouble, tbh. It takes millisecs and we need and internet connection to push the changes anyways.

When we're updating the package, sure. I run make update all the time though, and I would prefer that it didn't reach out to the internet unless needed (most of the time it would not). Please add a TODO -- I'm happy to do the work since it may only annoy me ;)

@codecov-io
Copy link

codecov-io commented Nov 24, 2020

Codecov Report

Merging #4432 (4d29307) into master (4727a8b) will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4432      +/-   ##
==========================================
- Coverage   76.11%   76.09%   -0.03%     
==========================================
  Files         158      158              
  Lines        9782     9782              
==========================================
- Hits         7446     7444       -2     
- Misses       2336     2338       +2     
Impacted Files Coverage Δ
beater/jaeger/common.go 75.00% <0.00%> (-7.15%) ⬇️
...ack/apm-server/aggregation/txmetrics/aggregator.go 93.36% <0.00%> (ø)

@jalvz
Copy link
Contributor Author

jalvz commented Nov 24, 2020

Wrote some actual docs in 2411077. Ill hand that off to Brandon for polishing and completing. I guess he will move that somewhere else and link instead; but it would be great a review now to make sure they are accurate and understandable.

Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with the additional x-pack fields. Thanks!

I haven't been over it with a fine-tooth comb, but it looks good and safe - so we can adjust over time as we test if needed.

apmpackage/cmd/gen-package/genfields.go Outdated Show resolved Hide resolved
apmpackage/cmd/gen-package/genfields.go Outdated Show resolved Hide resolved
@jalvz jalvz merged commit 576186d into elastic:master Nov 25, 2020
jalvz added a commit to jalvz/apm-server that referenced this pull request Dec 14, 2020
Adds an APM package for Fleet, with a script to generate fields.yml files and docs
# Conflicts:
#	go.mod
#	include/fields.go
axw pushed a commit that referenced this pull request Dec 15, 2020
Adds an APM package for Fleet, with a script to generate fields.yml files and docs
@simitt
Copy link
Contributor

simitt commented Dec 16, 2020

@jalvz not sure this needs additional manual testing - please feel free to remove the labels in case you don't think it brings value.

@simitt
Copy link
Contributor

simitt commented Dec 23, 2020

Loooked into how to test this and decided to add a test-plan-skip label as everything that has been generated via make update has been committed and therefore approved already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants